Skip to content

Migrate build script to Swift #1618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Apr 28, 2023

Fixes #1465

@kimdv
Copy link
Contributor Author

kimdv commented Apr 28, 2023

Open PR to show the progress.

I'm already open for feedback but it's still WIP

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch 4 times, most recently from 78950c9 to fc012b1 Compare May 3, 2023 14:57
@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch 2 times, most recently from 6bcc786 to 1b6ebb2 Compare May 7, 2023 19:25
@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from ebfed09 to 3d0cc38 Compare May 7, 2023 19:47
@ahoppen
Copy link
Member

ahoppen commented May 8, 2023

One high-level comment: I think the build script should live in the CodeGeneration package (which would then obviously need to be renamed). That way we could also remove the generate-swiftsyntax executable and instead invoke the source generation code directly from the rewritten build-script.

I would like to keep swift-parser-cli separate from the build script because they serve to drastically different purposes. The build script’s purpose is to build SwiftSyntax in CI and provide the glue code with the build script to the build-script in the compiler repo. As such, it’s only interesting during build time. swift-parser-cli is a runtime tool that can be super useful to inspect SwiftSyntax trees and e.g. triage bugs. For example, if someone is hitting a SwiftSyntax crash but can’t share their source code, you might suggest that they run swift-parser-cli reduce and just attach the reduced source code (which with all likelihood contains no sensitive information). As such, I see swift-parser-cli also useful for adopters of SwiftSyntax.

@kimdv
Copy link
Contributor Author

kimdv commented May 8, 2023

One high-level comment: I think the build script should live in the CodeGeneration package (which would then obviously need to be renamed). That way we could also remove the generate-swiftsyntax executable and instead invoke the source generation code directly from the rewritten build-script.

Cool. I'll try to merge CodeGeneration package and this one into a single Package.

I would like to keep swift-parser-cli separate from the build script because they serve to drastically different purposes. The build script’s purpose is to build SwiftSyntax in CI and provide the glue code with the build script to the build-script in the compiler repo. As such, it’s only interesting during build time. swift-parser-cli is a runtime tool that can be super useful to inspect SwiftSyntax trees and e.g. triage bugs. For example, if someone is hitting a SwiftSyntax crash but can’t share their source code, you might suggest that they run swift-parser-cli reduce and just attach the reduced source code (which with all likelihood contains no sensitive information). As such, I see swift-parser-cli also useful for adopters of SwiftSyntax.

Just to be clear, I would revert the commit or will you still move it out of the swift-syntax package but just in it's own package? (As now. It is it's own executable)
My tought was to move it out, so the swift-syntax package don't have any dependency on swift-argument-parser.

@ahoppen
Copy link
Member

ahoppen commented May 8, 2023

Just to be clear, I would revert the commit or will you still move it out of the swift-syntax package but just in it's own package? (As now. It is it's own executable) My tought was to move it out, so the swift-syntax package don't have any dependency on swift-argument-parser.

Yes, I think moving swift-parser-cli to a different package does make sense. I was just thrown off by the PR title and thought that swift-parser-cli would be the vehicle for migrating build-script and that’s definitely not what we want.

@kimdv
Copy link
Contributor Author

kimdv commented May 8, 2023

Yes, I think moving swift-parser-cli to a different package does make sense. I was just thrown off by the PR title and thought that swift-parser-cli would be the vehicle for migrating build-script and that’s definitely not what we want.

Will remove this PR so it focus on migrate the build script only then

@ahoppen
Copy link
Member

ahoppen commented May 8, 2023

I think a separate PR for moving swift-parser-cli would be great, but that’s a different topic.

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch 4 times, most recently from d12a844 to 5789a5c Compare May 8, 2023 18:54
@kimdv
Copy link
Contributor Author

kimdv commented May 8, 2023

The CLI look like

OVERVIEW: Build and test script for SwiftSyntax.

The build script can also drive the test suite included in the SwiftSyntax
repo. This requires a custom build of the compiler project since it accesses
test utilities that are not shipped as part of the toolchains. See the Testing
section for arguments that need to be specified for this.

USAGE: swift-syntax-cli <subcommand>

OPTIONS:
  -h, --help              Show help information.

SUBCOMMANDS:
  build
  generate-source-code
  test
  verify-source-code

  See 'swift-syntax-cli help <subcommand>' for detailed help.
Build help
USAGE: swift-syntax-cli build <toolchain> [<build-dir>] [<multiroot-data-file>] [--disable-sandbox] [--release] [--enable-rawsyntax-validation] [--enable-test-fuzzing] [--verbose]

ARGUMENTS:
  <toolchain>             The path to the toolchain that shall be used to build SwiftSyntax.
  <build-dir>             The directory in which build products shall be put. If omitted
                          a directory named ".build" will be put in the swift-syntax
                          directory. (default: .build)
  <multiroot-data-file>   Path to an Xcode workspace to create a unified build of SwiftSyntax with
                          other projects.

OPTIONS:
  --disable-sandbox       Disable sandboxes when building with SwiftPM
  --release               Build in release mode.
  --enable-rawsyntax-validation
                          When constructing RawSyntax nodes validate that their layout matches that
                          defined in `CodeGeneration` and that TokenSyntax nodes have a `tokenKind`
                          matching the ones specified in `CodeGeneration`.
  --enable-test-fuzzing   For each `assertParse` test, perform mutations of the test case based on
                          alternate token choices that the parser checks, validating that there are
                          no round-trip or assertion failures.
  --verbose               Enable verbose logging.
  -h, --help              Show help information.
Generate source code help
USAGE: swift-syntax-cli generate-source-code <destination> [--verbose]

ARGUMENTS:
  <destination>           The path to the source directory (i.e. 'swift-syntax/Sources') where the source files are to be generated

OPTIONS:
  --verbose               Enable verbose logging.
  -h, --help              Show help information.
Test help
USAGE: swift-syntax-cli test <toolchain> [<build-dir>] [<multiroot-data-file>] [--disable-sandbox] [--release] [--enable-rawsyntax-validation] [--enable-test-fuzzing] [--verbose] [--skip-lit-tests] [<filecheck-exec>]

ARGUMENTS:
  <toolchain>             The path to the toolchain that shall be used to build SwiftSyntax.
  <build-dir>             The directory in which build products shall be put. If omitted
                          a directory named ".build" will be put in the swift-syntax
                          directory. (default: .build)
  <multiroot-data-file>   Path to an Xcode workspace to create a unified build of SwiftSyntax with
                          other projects.
  <filecheck-exec>        Path to the FileCheck executable that was built as part of the LLVM repository.
                          If not specified, it will be looked up from PATH.

OPTIONS:
  --disable-sandbox       Disable sandboxes when building with SwiftPM
  --release               Build in release mode.
  --enable-rawsyntax-validation
                          When constructing RawSyntax nodes validate that their layout matches that
                          defined in `CodeGeneration` and that TokenSyntax nodes have a `tokenKind`
                          matching the ones specified in `CodeGeneration`.
  --enable-test-fuzzing   For each `assertParse` test, perform mutations of the test case based on
                          alternate token choices that the parser checks, validating that there are
                          no round-trip or assertion failures.
  --verbose               Enable verbose logging.
  --skip-lit-tests        Don't run lit-based tests
  -h, --help              Show help information.
Generate source code help
USAGE: swift-syntax-cli verify-source-code <destination> [--verbose]

ARGUMENTS:
  <destination>           The path to the source directory (i.e. 'swift-syntax/Sources') where the source files are to be generated

OPTIONS:
  --verbose               Enable verbose logging.
  -h, --help              Show help information.

@kimdv kimdv marked this pull request as ready for review May 8, 2023 19:04
@kimdv kimdv requested a review from ahoppen as a code owner May 8, 2023 19:04
@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch 3 times, most recently from 1e8939b to 529be49 Compare May 8, 2023 19:06
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this, @kimdv. I think the Swift implementation is really so much easier to read and work with. I’ve got a few comments though. Just to warn you, once you’ve addressed those, I might come up with another big-ish set of comments, but we’re definitely heading in the right direction.

@ahoppen
Copy link
Member

ahoppen commented May 14, 2023

One thought just came to me: With the current design, you won't be able to build the build script (or swift-Syntax-cli or whatever the name ends up being) if SwiftSyntax doesn't build itself, because the build script depends on SwiftSyntax. I think that's quite unfortunate and very surprising.

With that in mind, I think CodeGeneration will need to continue being its own package so that the build script itself can continue to be free of a SwiftSyntax dependency.

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch 3 times, most recently from 0da0cc5 to d7b3c42 Compare May 23, 2023 19:11
@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from e68d010 to 0052d81 Compare August 4, 2023 14:57
@kimdv
Copy link
Contributor Author

kimdv commented Aug 4, 2023

swiftlang/swift#66573

@swift-ci please test

2 similar comments
@kimdv
Copy link
Contributor Author

kimdv commented Aug 4, 2023

swiftlang/swift#66573

@swift-ci please test

@kimdv
Copy link
Contributor Author

kimdv commented Aug 4, 2023

swiftlang/swift#66573

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from 0052d81 to 539d80b Compare August 5, 2023 12:56
@kimdv
Copy link
Contributor Author

kimdv commented Aug 5, 2023

swiftlang/swift#66573

@swift-ci please test

@ahoppen
Copy link
Member

ahoppen commented Aug 5, 2023

To debug the latest failure (and in general), I think it would be good if swift-syntax-dev-utils would print the commands for all subprocesses it runs if it has --verbose, similar to

https://github.com/apple/swift-syntax/blob/861333cd66fa80e6dc0d0d11d0c05e218418e1b8/build-script.py#L95-L96C22

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from 539d80b to 8bfc4b4 Compare August 5, 2023 18:12
@kimdv
Copy link
Contributor Author

kimdv commented Aug 5, 2023

swiftlang/swift#66573

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from 8bfc4b4 to d3ed7e4 Compare August 6, 2023 09:54
@kimdv
Copy link
Contributor Author

kimdv commented Aug 6, 2023

swiftlang/swift#66573

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from d3ed7e4 to 894d0c5 Compare August 6, 2023 13:19
@kimdv
Copy link
Contributor Author

kimdv commented Aug 6, 2023

swiftlang/swift#66573

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch 2 times, most recently from 95a3b22 to cd1370a Compare August 7, 2023 07:37
@kimdv
Copy link
Contributor Author

kimdv commented Aug 7, 2023

swiftlang/swift#66573

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from cd1370a to c87486b Compare August 8, 2023 07:03
@kimdv
Copy link
Contributor Author

kimdv commented Aug 8, 2023

swiftlang/swift#66573

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from c87486b to f1471e8 Compare August 8, 2023 08:42
@kimdv
Copy link
Contributor Author

kimdv commented Aug 8, 2023

swiftlang/swift#66573

@swift-ci please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳 macOS is passing. Linux can’t be far

@kimdv kimdv force-pushed the kimdv/migrate-build-script-to-swift branch from f1471e8 to c148db3 Compare August 9, 2023 06:22
@kimdv
Copy link
Contributor Author

kimdv commented Aug 9, 2023

swiftlang/swift#66573

@swift-ci please test

@kimdv
Copy link
Contributor Author

kimdv commented Aug 9, 2023

swiftlang/swift#66573

@swift-ci please test windows

1 similar comment
@ahoppen
Copy link
Member

ahoppen commented Aug 9, 2023

swiftlang/swift#66573

@swift-ci please test windows

@kimdv kimdv merged commit ffe4c36 into swiftlang:main Aug 11, 2023
@kimdv kimdv deleted the kimdv/migrate-build-script-to-swift branch August 11, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite build-script.py in Swift
5 participants